[Feat] 계정 잠금 및 비밀번호 설정 규칙 #202
Conversation
riadan710
left a comment
riadan710
left a comment
There was a problem hiding this comment.
전반적으로 작동엔 문제가 없어 보이지만,
CustomUserDetailService에서
isLockedNow를 사용하는 잠금여부확인 및 처리 부분과,
passwordEncoder.matches를 사용하는 비밀번호 확인 부분은 별도 private 메소드로 빼는게 어떨까 싶습니다.
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
이번 PR은 로그인 실패 시 계정 잠금 기능과 새로운 비밀번호 정책을 도입하는 변경사항을 담고 있습니다. 전반적으로 올바르게 구현되었지만, 로그인 실패 처리 로직에 동시성 문제가 발생할 수 있는 부분이 있어 수정을 제안했습니다. 해당 부분은 여러 요청이 동시에 들어올 경우 시도 횟수가 정확하게 계산되지 않을 수 있습니다. 또한, 계정 잠금 시 사용자에게 표시되는 남은 시간 메시지를 더 명확하게 개선하는 방안도 제안했습니다. 비밀번호 유효성 검사 규칙과 로그인 처리에 대한 트랜잭션 설정은 올바르게 적용되었습니다.
| if (!passwordEncoder.matches(password, user.getPassword())) { | ||
| throw new BadCredentialsException("Password가 일치하지 않습니다."); | ||
| Users refreshed = userRepository.findByUsername(username).orElse(user); | ||
| int attempts = refreshed.getFailedAttempts(); | ||
| int currentAttempts = attempts + 1; | ||
|
|
||
| if (currentAttempts >= MAX_ATTEMPTS) { | ||
| refreshed.lockFor(LOCK_DURATION); | ||
| userRepository.save(refreshed); | ||
| throw new LockedException("비밀번호 " + MAX_ATTEMPTS + "회 오류로 계정 잠긴 상태입니다."); | ||
| } | ||
|
|
||
| user.updateFailedAttempts(currentAttempts); | ||
| userRepository.save(user); | ||
|
|
||
| throw new BadCredentialsException("아이디 또는 비밀번호가 올바르지 않습니다. (남은 시도: " + (MAX_ATTEMPTS - currentAttempts) + "회)"); | ||
| } |
There was a problem hiding this comment.
로그인 실패 시 사용자 정보를 업데이트하는 로직에 동시성 문제가 발생할 수 있으며, 코드가 복잡합니다. user와 refreshed라는 두 개의 다른 객체 인스턴스를 사용하여 데이터를 업데이트하고 있어, 동시 요청 시 데이터 불일치가 발생할 수 있습니다. 예를 들어, refreshed 객체로 잠금 여부를 확인하고 잠금을 설정하지만, 잠기지 않은 경우에는 원본 user 객체의 실패 횟수를 업데이트하고 저장합니다. 이로 인해 failedAttempts가 정확히 증가하지 않을 수 있습니다. 하나의 사용자 객체 인스턴스를 일관되게 사용하고, 데이터베이스 저장 연산을 한 번으로 줄여 코드를 더 명확하고 안전하게 만드는 것이 좋습니다.
if (!passwordEncoder.matches(password, user.getPassword())) {
// 동시 로그인 시도에 대한 경쟁 상태를 방지하기 위해 사용자 정보를 다시 가져옵니다.
Users userToUpdate = userRepository.findByUsername(username)
.orElseThrow(() -> new UsernameNotFoundException("해당 아이디를 가진 유저가 존재하지 않습니다: " + username));
int newAttempts = userToUpdate.getFailedAttempts() + 1;
userToUpdate.updateFailedAttempts(newAttempts);
if (newAttempts >= MAX_ATTEMPTS) {
userToUpdate.lockFor(LOCK_DURATION);
}
userRepository.save(userToUpdate);
if (newAttempts >= MAX_ATTEMPTS) {
throw new LockedException("비밀번호 " + MAX_ATTEMPTS + "회 오류로 계정 잠긴 상태입니다.");
} else {
throw new BadCredentialsException("아이디 또는 비밀번호가 올바르지 않습니다. (남은 시도: " + (MAX_ATTEMPTS - newAttempts) + "회)");
}
}| long remainHours = remain.toHours(); | ||
| long remainMinutes = remain.toMinutes(); | ||
|
|
||
| String message; | ||
| if (remainHours >= 1) { | ||
| message = "비밀번호 " + MAX_ATTEMPTS + "회 오류로 계정 잠긴 상태입니다. 약 " + (remainHours + 1) + "시간 후에 다시 시도해주세요."; | ||
| } else { | ||
| long remainMin = Math.max(1, remainMinutes); | ||
| message = "비밀번호 " + MAX_ATTEMPTS + "회 오류로 계정 잠긴 상태입니다. 약 " + remainMin + "분 후에 다시 시도해주세요."; | ||
| } |
There was a problem hiding this comment.
계정 잠금 시 남은 시간을 안내하는 메시지가 사용자에게 혼란을 줄 수 있습니다. 현재 로직은 남은 시간을 시간 단위로 올림하여 보여주기 때문에, 예를 들어 1시간 1분이 남았을 때와 1시간 59분이 남았을 때 모두 "약 2시간"으로 표시됩니다. 사용자에게 더 정확한 정보를 제공하기 위해 시간과 분을 함께 표시하는 것을 고려해보세요.
| long remainHours = remain.toHours(); | |
| long remainMinutes = remain.toMinutes(); | |
| String message; | |
| if (remainHours >= 1) { | |
| message = "비밀번호 " + MAX_ATTEMPTS + "회 오류로 계정 잠긴 상태입니다. 약 " + (remainHours + 1) + "시간 후에 다시 시도해주세요."; | |
| } else { | |
| long remainMin = Math.max(1, remainMinutes); | |
| message = "비밀번호 " + MAX_ATTEMPTS + "회 오류로 계정 잠긴 상태입니다. 약 " + remainMin + "분 후에 다시 시도해주세요."; | |
| } | |
| long remainHours = remain.toHours(); | |
| long remainMinutesPart = remain.toMinutes() % 60; | |
| String message; | |
| if (remainHours >= 1) { | |
| message = "비밀번호 " + MAX_ATTEMPTS + "회 오류로 계정 잠긴 상태입니다. 약 " + remainHours + "시간 " + remainMinutesPart + "분 후에 다시 시도해주세요."; | |
| } else { | |
| long remainMin = Math.max(1, remain.toMinutes()); | |
| message = "비밀번호 " + MAX_ATTEMPTS + "회 오류로 계정 잠긴 상태입니다. 약 " + remainMin + "분 후에 다시 시도해주세요."; | |
| } |
#️⃣연관된 이슈
📝작업 내용
🔎코드 설명
💬고민사항 및 리뷰 요구사항 (Optional)
비고 (Optional)